ENG-1719 List groups I'm a member of by maparent · Pull Request #1011 · DiscourseGraphs/discourse-graph

maparent · 2026-05-10T16:42:17Z

https://linear.app/discourse-graphs/issue/ENG-1719/list-groups-im-a-member-of

This creates a vercel page with a group list, which uses the database token set up in ENG-1703.

https://www.loom.com/share/1897670dcaae4908a3fde4ce4cf3c22e

linear-code · 2026-05-10T16:42:20Z

supabase · 2026-05-10T16:42:31Z

Updates to Preview Branch (eng-1719-list-groups-im-a-member-of) ↗︎

Deployments	Status	Updated
Database	✅	Thu, 21 May 2026 12:50:23 UTC
Services	✅	Thu, 21 May 2026 12:50:23 UTC
APIs	✅	Thu, 21 May 2026 12:50:23 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks	Status	Updated
Configurations	✅	Thu, 21 May 2026 12:50:25 UTC
Migrations	✅	Thu, 21 May 2026 12:50:25 UTC
Seeding	✅	Thu, 21 May 2026 12:50:25 UTC
Edge Functions	✅	Thu, 21 May 2026 12:50:30 UTC

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

graphite-app · 2026-05-13T11:11:09Z

PR size/scope check

This PR is over our review-size guideline.

Recommended: ~200 lines changed
Acceptable limit: up to 400 lines when well-scoped/self-contained
Preferred file count: fewer than 5 files

Please split this into smaller PRs unless there is a clear reason the changes need to land together.

If keeping it as one PR, please add a brief justification covering:

What single problem this PR solves
Why the files/changes are coupled

mdroidian

Codex PR Review

Findings

High: apps/website/app/components/auth/LoginWithToken.tsx should not perform the token exchange in a Client Component. The token is removed from the URL only after hydration via useEffect, but PostHogPageView reads searchParams and can capture $current_url before that happens. That risks leaking the one-time token to analytics.

High: router.replace(url) uses a query-derived url without validation. Next.js warns against passing unsanitized URLs to router.push/router.replace. This should be restricted to same-origin relative paths, or default to /auth/group.

Medium: ListGroups.tsx does not appear to need "use client". It is session-aware data fetching plus rendering, and the app already has a cookie-backed Supabase server client. This should be an async Server Component instead of fetching in useEffect.

Recommendation

I would keep client code only where browser APIs are genuinely required. In this PR, /auth/group should be server-rendered:

Use ~/utils/supabase/server in an async Server Component.
Fetch the current user, groups, and memberships on the server.
Render empty/not-logged-in states directly.
Use a colocated error.tsx for unexpected failures. The error boundary itself must be client-side, but that keeps "use client" isolated to error UI.

For /auth/token, I would avoid a Client Component entirely if possible. A Route Handler is a better fit:

Read t and url from the request.
Validate the token payload.
Call auth.setSession using the server Supabase client so cookies are set in the response.
Redirect to a sanitized internal path.
Avoid exposing the token to hydration, analytics, or browser history.

So: we do need "use client" for a Next error boundary, and we would need it if we keep useSearchParams, useRouter, useEffect, or window.history. But for this flow, the better Next.js-specific approach is Server Components plus a Route Handler, not expanding the client boundary.

mdroidian

Sorry, it doesn't look like the issues in the review comment were addressed. Could you address those please?

maparent · 2026-05-16T14:21:12Z

Ok, I did not see it. After research, I see it is possible to rewrite all that code as server components, but setting that up is a totally distinct task. I'll create it.

mdroidian · 2026-05-17T01:50:03Z

Could you elaborate on why it is a distinct task?

I see it as the recommended way to achieve the current task (using the NextJS recommended way to fetch and render data)

maparent · 2026-05-17T01:56:39Z

I had to rewrite the token route to work server-side. It's a big PR, I'll try to think of ways to break it up tomorrow.

maparent · 2026-05-20T14:54:58Z

We are now using the backend client. Added a test.

maparent · 2026-05-20T15:02:09Z

Here I'm more embarrassed. Again, the PR is over size; a lot of that is due to testing. I do think it would be counter-productive to separate tests in another PR. I do not think it would be very helpful to split this PR in another way, though I could separate account.ts+tests, then all UI. Would that actually be helpful? But I think a generic argument should be made that tests need not the same level of review than the rest of the code, and should actually reduce the review burden; and that their LoC should be counted separately. What do you think?

mdroidian · 2026-05-20T15:09:48Z

Here I'm more embarrassed. Again, the PR is over size; a lot of that is due to testing. I do think it would be counter-productive to separate tests in another PR. I do not think it would be very helpful to split this PR in another way, though I could separate account.ts+tests, then all UI. Would that actually be helpful? But I think a generic argument should be made that tests need not the same level of review than the rest of the code, and should actually reduce the review burden; and that their LoC should be counted separately. What do you think?

The documented limit is:

Recommended: ~200 lines changed
Acceptable limit: up to 400 lines when well-scoped/self-contained

This PR is +281 -3
Lines changed: 281 additions & 3 deletions

I see that as falling within

Acceptable limit: up to 400 lines when well-scoped/self-contained

maparent · 2026-05-20T15:18:56Z

Fair. But I think the question of test size needs to be addressed separately.

mdroidian · 2026-05-20T21:22:42Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b561143b28

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-20T21:26:26Z

+              {groupData.map((d) => (
+                <li key={d.id}>
+                  {adminData[d.id || ""] ? (
+                    <Link href={"group/" + d.id!}>{d.name}</Link>


Prefix group link with root-relative path

Use a root-relative URL here (for example "/group/" + d.id) instead of "group/" + d.id. In Next.js, this current value is resolved relative to the current page (/auth/group), so admins clicking a group name are sent to /auth/group/group/<id> rather than the intended group route, which results in a broken navigation path for the primary admin action on this screen.

Useful? React with 👍 / 👎.

mdroidian

Also see the codex comment

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch 3 times, most recently from 11ba9fb to bb44e58 Compare May 11, 2026 20:38

graphite-app Bot reviewed May 12, 2026

View reviewed changes

Comment thread apps/website/app/utils/supabase/account.ts

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch 2 times, most recently from a8e2660 to 042ddb9 Compare May 13, 2026 16:26

maparent requested a review from mdroidian May 13, 2026 16:31

maparent mentioned this pull request May 13, 2026

ENG-1723 backend function: List group members #1026

Merged

graphite-app Bot reviewed May 13, 2026

View reviewed changes

Comment thread apps/website/app/components/auth/ListGroups.tsx Outdated

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 85424fa to 042ddb9 Compare May 13, 2026 18:12

mdroidian requested changes May 15, 2026

View reviewed changes

Comment thread apps/website/app/components/auth/ListGroups.tsx Outdated

Comment thread apps/website/app/components/auth/ListGroups.tsx Outdated

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 6326236 to 59041da Compare May 15, 2026 13:37

maparent changed the base branch from main to eng-1758-vitest-infrastructure May 15, 2026 13:39

maparent requested a review from mdroidian May 15, 2026 13:39

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 59041da to 97f32fb Compare May 15, 2026 13:41

maparent changed the base branch from eng-1758-vitest-infrastructure to main May 15, 2026 13:41

mdroidian requested changes May 15, 2026

View reviewed changes

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 97f32fb to 1006c5b Compare May 17, 2026 19:13

graphite-app Bot reviewed May 17, 2026

View reviewed changes

Comment thread apps/website/app/components/auth/ListGroups.tsx Outdated

Comment thread apps/website/app/(home)/auth/token/route.ts

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 1006c5b to 319e2c6 Compare May 17, 2026 19:36

maparent changed the base branch from main to eng-1758-vitest-infrastructure May 17, 2026 19:39

maparent force-pushed the eng-1758-vitest-infrastructure branch from 4a8f9bd to 8fde609 Compare May 18, 2026 11:20

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 319e2c6 to dea6c28 Compare May 18, 2026 11:21

maparent force-pushed the eng-1758-vitest-infrastructure branch from 8fde609 to a5c951e Compare May 18, 2026 13:33

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from dea6c28 to f3988c5 Compare May 18, 2026 13:48

maparent force-pushed the eng-1758-vitest-infrastructure branch from a5c951e to 66a8fe4 Compare May 18, 2026 16:01

Base automatically changed from eng-1758-vitest-infrastructure to main May 18, 2026 19:39

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from f3988c5 to 7d5a53a Compare May 18, 2026 19:43

maparent added 4 commits May 19, 2026 12:26

ENG-1917: List groups

a8c3953

replace a by Link

d35722e

review nits

868bac6

Rewritten as SSR

c07b6d5

maparent force-pushed the eng-1719-list-groups-im-a-member-of branch from 7d5a53a to c07b6d5 Compare May 19, 2026 16:27

maparent added 2 commits May 20, 2026 10:33

variable shadow and lint

00b769d

another instance of typo

1294a8d

maparent requested review from mdroidian and removed request for mdroidian May 20, 2026 14:38

test list my groups

b561143

maparent requested a review from mdroidian May 20, 2026 14:55

chatgpt-codex-connector Bot reviewed May 20, 2026

View reviewed changes

mdroidian approved these changes May 21, 2026

View reviewed changes

Comment thread apps/website/app/(home)/auth/group/page.tsx

mdroidian reviewed May 21, 2026

View reviewed changes

Comment thread apps/website/test/integration/listMyGroups.test.ts

maparent added 2 commits May 21, 2026 08:43

rename test file

fe1af4b

absolute link correction

77d236d

maparent merged commit 2b08704 into main May 21, 2026
12 checks passed

maparent deleted the eng-1719-list-groups-im-a-member-of branch May 21, 2026 12:51

Uh oh!

Conversation

maparent commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear-code Bot commented May 10, 2026

Uh oh!

supabase Bot commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

graphite-app Bot commented May 13, 2026

PR size/scope check

Uh oh!

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Codex PR Review

Findings

Recommendation

Uh oh!

Uh oh!

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

maparent commented May 16, 2026

Uh oh!

mdroidian commented May 17, 2026

Uh oh!

maparent commented May 17, 2026

Uh oh!

Uh oh!

Uh oh!

maparent commented May 20, 2026

Uh oh!

maparent commented May 20, 2026

Uh oh!

mdroidian commented May 20, 2026

Uh oh!

maparent commented May 20, 2026

Uh oh!

mdroidian commented May 20, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maparent commented May 10, 2026 •

edited

Loading

supabase Bot commented May 10, 2026 •

edited

Loading